Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

export/import_nir__Adding-Conv2D_and_AvgPool2d #305

Merged
merged 12 commits into from
Jul 18, 2024

Conversation

SirineArfa
Copy link
Contributor

  • fixes Missing Conv2D and AveragePooling2D modules in export/import to NIR #304
  • Adding support for the following torch.nn modules [Conv2d, AvgPool2d and Flatten] to export_nir.py and import_nir.py
  • Create example notebook in examples/tutorial_snntorch_to_nir.ipynb for snntorch model with layers [Conv2d, AvgPool2d,LIF, Linear,LIF] and export/import it to NIR
  • Include this model's tests in tests/test_nir.py

@SirineArfa
Copy link
Contributor Author

  • The pip version of NIR is not automatically updated and is currently version 1.0.1 from Dec 25. Hence, it does not contain AvgPool2d yet.

  • To try this feature you must build NIR from source:
    - git clone https://github.com/neuromorphs/NIR.git
    - cd NIR
    - pip install .

@asti205
Copy link

asti205 commented Jul 14, 2024

Yep, this would be really practical to have (pls review) :)

@jeshraghian
Copy link
Owner

There are a couple of conflicts in snntorch/functional/loss.py.

  • Line 100: arguments are different; population coding is missing.
  • Line 114: weights are now an instance variable; I'm not totally sure if these are inherited properly.

@SirineArfa
Copy link
Contributor Author

SirineArfa commented Jul 16, 2024

Conflicts in snntorch/functional/loss.py were resolved by merging the changes from master branch to Adding-Conv2D_and_Pooling branch because the original changes for export/import_nir__Adding-Conv2D_and_AvgPool2d were not not intended to affect the loss function logic.

@jeshraghian
Copy link
Owner

The tests haven't passed as there is now a dependency on sinabs.layers.
I don't think we should add dependencies on external libraries to enable NIR exports to function correctly.
How easy would it be to remove that requirement?

@SirineArfa
Copy link
Contributor Author

The sinabs.layers dependencies for sumpooling layers were easily removed, as this feature was used for a custom model conversion to NIR and indeed shouldn't be added to the snntorch NIR functions.

@jeshraghian
Copy link
Owner

Thank you! I ran the tests and it oddly worked with 3.9/3.10, but it didn't work on Python 3.8.
One of the tests throws module 'nir' has no attribute 'AvgPool2d'.

I'm not totally sure why this wouldn't be compatible with 3.8...

@stevenabreu7 : have you observed anything like this among the other libraries?

I would rather not eliminate compatibility with 3.8 over this, but a non-ideal option is to modify the test if we're not sure why this error persists.

@SirineArfa
Copy link
Contributor Author

I checked the NIR repo and in the pyproject.toml file it indicates in line 35 requires-python = ">=3.9" and I think that's why the tests in snntorch for python 3.8 fails, so I guess if it is the case, then the snntorch compatibility with 3.8 should be eliminated.

@jeshraghian
Copy link
Owner

thanks for checking that. Yep, let's remove Python 3.8 from the test matrix. I tried to commit to your fork but I don't have the necessary permissions. All that needs to be changed are the following files:

  • .github/workflows/build-tag.yml
  • CONTRIBUTING.rst
  • setup.py

If you update those, the tests should pass and I'll merge the PR.

@SirineArfa
Copy link
Contributor Author

I removed python 3.8 dependencies from the appointed files.

@jeshraghian jeshraghian merged commit 398c7c4 into jeshraghian:master Jul 18, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Conv2D and AveragePooling2D modules in export/import to NIR
3 participants